[Level Events] manage level events registration mask #13787

davinci26 · 2020-10-27T22:09:27Z

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com
Fixes: #13189

In this PR we introduce FileTriggerType::EmulatedEdge which are level events that behave similarly to edge events. We achieve this behavior in the following way:

When a socket operation would block we activate the READ | Write registration mark (depending on the operation).
When a socket event is activated we disable the READ | Write registration mark.
When SSL shutdowns we force read

With this change, we manage to enable the remaining failing tests on Windows.

Additional Description:

We expect to have roughly the same performance on Windows and on Linux.

Risk Level: Low (Windows Only)
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

include/envoy/event/file_event.h

antoniovicente

This is a really interesting and promising change. Here are some initial thoughts.

source/common/network/connection_impl.cc

source/common/network/io_socket_handle_impl.cc

source/common/network/connection_impl.cc

wrowe

Have a look at your //test/... compilation errors, but otherwise this is looking extremely promising, some tests we enabled which appear to no longer be flaky_on_windows...

//test/extensions/filters/http/ratelimit:ratelimit_integration_test      PASSED in 30.7s
//test/extensions/stats_sinks/metrics_service:metrics_service_integration_test PASSED in 8.3s
//test/integration:http_subset_lb_integration_test                       PASSED in 8.9s
//test/integration:tcp_tunneling_integration_test                        PASSED in 18.3s
//test/integration:websocket_integration_test                            PASSED in 11.8s
//test/integration:hds_integration_test                                  PASSED in 33.8s
  Stats over 2 runs: max = 33.8s, min = 32.9s, avg = 33.4s, dev = 0.4s
//test/integration:idle_timeout_integration_test                         PASSED in 41.4s
  Stats over 2 runs: max = 41.4s, min = 40.8s, avg = 41.1s, dev = 0.3s
//test/integration:http2_integration_test                                PASSED in 17.4s
  Stats over 4 runs: max = 17.4s, min = 12.9s, avg = 14.9s, dev = 1.7s
//test/integration:protocol_integration_test                             PASSED in 45.3s
  Stats over 5 runs: max = 45.3s, min = 34.6s, avg = 39.9s, dev = 3.6s

davinci26 · 2020-10-29T21:29:54Z

@wrowe I am trying to change the implementation to move the registration/unregistration logic to the file_event_impl as suggested.

In terms of testing I think I will try to do a CI run on Linux with FORCE_LEVEL_EVENTS and see the integration tests there.

sunjayBhatia · 2020-10-29T21:44:19Z

@davinci26 have you considered transforming this logic:

if (Event::optimizeLevelEvents &&
      Event::PlatformDefaultTriggerType == Event::FileTriggerType::Level) {
    if (!result.ok() && result.err_->getErrorCode() == Api::IoError::IoErrorCode::Again) {
       ...

into a macro or helper function etc? (could it be some case evaluation done using compiler defines so theres not so many if checks/use constexpr if? also so we don't have to rewrite it many times and can avoid copy/paste errors)

is this part of the logic you are looking to move closer to the file event impl or just the bits around setting the event mask:

ioHandle().enableFileEvents(ioHandle().getEnabledFileEvents() & ~Event::FileReadyType::Read);

davinci26 · 2020-10-29T21:48:48Z

@sunjayBhatia I have changed this locally but I need to do some more changes in my new implementation.

include/envoy/event/file_event.h

source/common/event/file_event_impl.cc

davinci26 · 2020-10-30T19:03:40Z

@antoniovicente I addressed all your comments and added the EmulatedEdge type as discussed offline. I think this should be ready for another review

davinci26 · 2020-10-30T19:04:31Z

I still need to work through some changes in the ssl_socket, I dont think we handle well the case were EAWOULDBLOCK is the error during the handshake.

source/common/event/file_event_impl.cc

antoniovicente

I still need to work through some changes in the ssl_socket, I dont think we handle well the case were EAWOULDBLOCK is the error during the handshake.

Can you do a master merge to pick up #13772 ?

source/common/event/file_event_impl.cc

test/test_common/threadsafe_singleton_injector.h

test/common/network/connection_impl_test.cc

nigriMSFT · 2020-11-03T05:47:06Z

include/envoy/event/file_event.h

@@ -18,15 +18,28 @@ struct FileReadyType {
  static const uint32_t Closed = 0x4;
 };

-enum class FileTriggerType { Level, Edge };
+#define FORCE_LEVEL_EVENTS 0
+enum class FileTriggerType { Level, Edge, EmulatedEdge };


Can we get rid of Level now? Windows is the only platform its needed and we fully intend to use the "optimized" flavor anyhow. Or are you using it for testing/comparison?

Level events are used by listeners and DNS ( on all platforms), so we cant really remove it.

Ah I did not know that. Have you played with trying to move listeners and DNS to EmulatedEdge? Just curious. This is something that can certainly be done later if there is interest

Can you add some comments now above each trigger type with what they do and what they are used for? I think now it's less clear what all of these do and are for and it would be good to have a comment trail.

I added a block of comments for EmulatedEdge and some comments with the current use cases of the other event types

source/common/event/file_event_impl.cc

source/common/network/io_socket_handle_impl.cc

davinci26 · 2020-11-04T17:19:18Z

@antoniovicente ready for a review.

The changes I made are:

removed EVLOOP_ONCE flag.
Now I no longer cancel injected events when modifying the registration.
Handle some edge cases with SSL and early close

davinci26 · 2020-11-05T17:51:43Z

Remove fails_on_windows from active_quic_listener_test (see #13918)

include/envoy/event/file_event.h

source/common/event/file_event_impl.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

antoniovicente

I think that's it. Change looks great except for the minor comments below.

include/envoy/event/file_event.h

source/common/event/file_event_impl.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

antoniovicente

Off to @envoyproxy/senior-maintainers for final review.

Awesome improvement to event handling on Windows. Some further refinements would be good but it's better to tackle them over time. Do share with me performance data and plans for further improvements.

Thank you for structuring the change so it is minimally intrusive on other platforms while also allowing compile time defines to opt into this mode for debugging.

davinci26 · 2020-11-19T17:51:27Z

@envoyproxy/senior-maintainers note that because the change is guarded with constexpr if to avoid having perf penalty on POSIX systems I have dropped the line coverage substantially. Even though the code inside the constexpr if is not running in POSIX and it should never run it counts towards LCOV.

This code is impossible to test on POSIX unless:

We have a new compilation unit that has the FORCE_LEVEL_EVENTS
Make this a runtime feature and toggle it on for tests so this feature gets executed. Given that this code is already executed on Windows and there is no plan to have it on POSIX I am not sure if this adds significant value.

Let me know what you think and I will implement it!

antoniovicente · 2020-11-19T18:00:43Z

@envoyproxy/senior-maintainers note that because the change is guarded with constexpr if to avoid having perf penalty on POSIX systems I have dropped the line coverage substantially. Even though the code inside the constexpr if is not running in POSIX and it should never run it counts towards LCOV.

This code is impossible to test on POSIX unless:

We have a new compilation unit that has the FORCE_LEVEL_EVENTS

Make this a runtime feature and toggle it on for tests so this feature gets executed. Given that this code is already executed on Windows and there is no plan to have it on POSIX I am not sure if this adds significant value.

Let me know what you think and I will implement it!

I would suggest you adjust the coverage expectations. Some low level tests that cover basic adjustments to the event mask after events are delivered and after readv calls that block would be good to have when emulated edge is enabled.

mattklein123

This is very clever. Nice work! Just some small comments. I agree with @antoniovicente about changing coverage expectations. I suppose one option would be to have some test under coverage that is compiled a 2nd time with the force enable flag for the feature? I'm not sure how hard that would be. Thank you!

/wait

mattklein123 · 2020-11-19T18:19:12Z

include/envoy/event/file_event.h

@@ -18,15 +18,28 @@ struct FileReadyType {
  static const uint32_t Closed = 0x4;
 };

-enum class FileTriggerType { Level, Edge };
+#define FORCE_LEVEL_EVENTS 0
+enum class FileTriggerType { Level, Edge, EmulatedEdge };


Can you add some comments now above each trigger type with what they do and what they are used for? I think now it's less clear what all of these do and are for and it would be good to have a comment trail.

include/envoy/event/file_event.h

source/common/event/file_event_impl.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

mattklein123

Thanks!

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

antoniovicente

Waiting for CI to finish.

I'm very glad we got to the end of this awesome improvement.

davinci26 · 2020-11-19T23:17:08Z

@antoniovicente thanks a ton for the reviews and all the help with landing this! Your help was really crucial.

@envoyproxy/senior-maintainers thanks for your support to make Envoy cross platform.

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> Signed-off-by: Qin Qin <qqin@google.com>

davinci26 assigned antoniovicente Oct 27, 2020

davinci26 commented Oct 27, 2020

View reviewed changes

include/envoy/event/file_event.h Outdated Show resolved Hide resolved

antoniovicente reviewed Oct 27, 2020

View reviewed changes

antoniovicente mentioned this pull request Oct 28, 2020

Io socket handle for internal socket #13418

Closed

wrowe reviewed Oct 29, 2020

View reviewed changes

davinci26 commented Oct 30, 2020

View reviewed changes

include/envoy/event/file_event.h Outdated Show resolved Hide resolved

davinci26 commented Oct 30, 2020

View reviewed changes

include/envoy/event/file_event.h Outdated Show resolved Hide resolved

davinci26 commented Oct 30, 2020

View reviewed changes

source/common/event/file_event_impl.cc Outdated Show resolved Hide resolved

davinci26 commented Oct 30, 2020

View reviewed changes

source/common/event/file_event_impl.cc Outdated Show resolved Hide resolved

davinci26 commented Oct 30, 2020

View reviewed changes

source/common/event/file_event_impl.cc Outdated Show resolved Hide resolved

davinci26 mentioned this pull request Nov 1, 2020

Envoy is spinning CPU on Windows #13189

Closed

antoniovicente reviewed Nov 2, 2020

View reviewed changes

source/common/event/file_event_impl.cc Outdated Show resolved Hide resolved

davinci26 force-pushed the eventsOptimized branch from d6eca0e to 588fff8 Compare November 2, 2020 23:31

davinci26 commented Nov 3, 2020

View reviewed changes

test/test_common/threadsafe_singleton_injector.h Outdated Show resolved Hide resolved

davinci26 commented Nov 3, 2020

View reviewed changes

test/common/network/connection_impl_test.cc Outdated Show resolved Hide resolved

nigriMSFT reviewed Nov 3, 2020

View reviewed changes

source/common/event/file_event_impl.cc Outdated Show resolved Hide resolved

nigriMSFT reviewed Nov 3, 2020

View reviewed changes

source/common/network/io_socket_handle_impl.cc Outdated Show resolved Hide resolved

nigriMSFT reviewed Nov 3, 2020

View reviewed changes

source/common/network/io_socket_handle_impl.cc Outdated Show resolved Hide resolved

davinci26 mentioned this pull request Nov 3, 2020

Support level trigger user space event and resolve fails_on_windows introduced in #13418 #13865

Open

davinci26 mentioned this pull request Nov 5, 2020

Windows: Fix active_quic_listener_test #13918

Closed

davinci26 marked this pull request as ready for review November 5, 2020 18:12

davinci26 requested a review from asraa as a code owner November 5, 2020 18:12

antoniovicente reviewed Nov 18, 2020

View reviewed changes

include/envoy/event/file_event.h Outdated Show resolved Hide resolved

include/envoy/event/file_event.h Outdated Show resolved Hide resolved

source/common/event/file_event_impl.cc Outdated Show resolved Hide resolved

Sotiris Nanopoulos added 2 commits November 18, 2020 12:48

fix pr comments

342fe73

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

optimize events for winodws

ee2b6e0

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

antoniovicente reviewed Nov 18, 2020

View reviewed changes

include/envoy/event/file_event.h Outdated Show resolved Hide resolved

source/common/event/file_event_impl.cc Outdated Show resolved Hide resolved

source/common/event/file_event_impl.cc Show resolved Hide resolved

Sotiris Nanopoulos added 4 commits November 18, 2020 15:01

More constexpr guarding

da34cb0

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Merge remote-tracking branch 'upstream/master' into eventsOptimized

54bff41

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix merge conflict

5d135e9

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

more constexpr guards vol2

a730507

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

antoniovicente previously approved these changes Nov 19, 2020

View reviewed changes

mattklein123 requested changes Nov 19, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 19, 2020

mattklein123 self-assigned this Nov 19, 2020

Sotiris Nanopoulos added 2 commits November 19, 2020 11:26

PR comments by Matt and per file coverage

bc4278d

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

spelling

afb62cb

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 dismissed antoniovicente’s stale review via afb62cb November 19, 2020 19:27

repokitteh-read-only bot removed the waiting label Nov 19, 2020

Sotiris Nanopoulos added 2 commits November 19, 2020 11:42

expand comments on event types

48f9376

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

spelling

bf30da3

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

mattklein123 previously approved these changes Nov 19, 2020

View reviewed changes

drop coverage for events to 93.5

3e9ca9c

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 dismissed mattklein123’s stale review via 3e9ca9c November 19, 2020 22:18

antoniovicente approved these changes Nov 19, 2020

View reviewed changes

mattklein123 merged commit 11dee4e into envoyproxy:master Nov 20, 2020

andreyprezotto pushed a commit to andreyprezotto/envoy that referenced this pull request Nov 24, 2020

[Level Events] manage level events registration mask (envoyproxy#13787)

e73172e

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

qqustc pushed a commit to qqustc/envoy that referenced this pull request Nov 24, 2020

[Level Events] manage level events registration mask (envoyproxy#13787)

7338588

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> Signed-off-by: Qin Qin <qqin@google.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Level Events] manage level events registration mask #13787

[Level Events] manage level events registration mask #13787

davinci26 commented Oct 27, 2020 •

edited

Loading

antoniovicente left a comment

wrowe left a comment

davinci26 commented Oct 29, 2020

sunjayBhatia commented Oct 29, 2020 •

edited

Loading

davinci26 commented Oct 29, 2020

davinci26 commented Oct 30, 2020

davinci26 commented Oct 30, 2020

antoniovicente left a comment

nigriMSFT Nov 3, 2020

davinci26 Nov 3, 2020

nigriMSFT Nov 3, 2020

mattklein123 Nov 19, 2020

davinci26 Nov 19, 2020

davinci26 commented Nov 4, 2020 •

edited

Loading

davinci26 commented Nov 5, 2020

antoniovicente left a comment

antoniovicente left a comment •

edited

Loading

davinci26 commented Nov 19, 2020

antoniovicente commented Nov 19, 2020

mattklein123 left a comment

mattklein123 Nov 19, 2020

mattklein123 left a comment

antoniovicente left a comment

davinci26 commented Nov 19, 2020

[Level Events] manage level events registration mask #13787

[Level Events] manage level events registration mask #13787

Conversation

davinci26 commented Oct 27, 2020 • edited Loading

antoniovicente left a comment

Choose a reason for hiding this comment

wrowe left a comment

Choose a reason for hiding this comment

davinci26 commented Oct 29, 2020

sunjayBhatia commented Oct 29, 2020 • edited Loading

davinci26 commented Oct 29, 2020

davinci26 commented Oct 30, 2020

davinci26 commented Oct 30, 2020

antoniovicente left a comment

Choose a reason for hiding this comment

nigriMSFT Nov 3, 2020

Choose a reason for hiding this comment

davinci26 Nov 3, 2020

Choose a reason for hiding this comment

nigriMSFT Nov 3, 2020

Choose a reason for hiding this comment

mattklein123 Nov 19, 2020

Choose a reason for hiding this comment

davinci26 Nov 19, 2020

Choose a reason for hiding this comment

davinci26 commented Nov 4, 2020 • edited Loading

davinci26 commented Nov 5, 2020

antoniovicente left a comment

Choose a reason for hiding this comment

antoniovicente left a comment • edited Loading

Choose a reason for hiding this comment

davinci26 commented Nov 19, 2020

antoniovicente commented Nov 19, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Nov 19, 2020

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

antoniovicente left a comment

Choose a reason for hiding this comment

davinci26 commented Nov 19, 2020

davinci26 commented Oct 27, 2020 •

edited

Loading

sunjayBhatia commented Oct 29, 2020 •

edited

Loading

davinci26 commented Nov 4, 2020 •

edited

Loading

antoniovicente left a comment •

edited

Loading